Directory currency - performance optimalization #1252

Sekiphp · 2020-10-07T22:22:24Z

Description (*)

Lowering count of SQL queries when someone use this methods:

getConfigAllowCurrencies
getConfigBaseCurrencies

Fix unique currency which is at the end of string for mthod getConfigAllowCurrencies.

Manual testing scenarios `getConfigBaseCurrencies` (*)

Call this code:

var_dump(Mage::getModel('directory/currency')->getConfigBaseCurrencies());
var_dump(Mage::getModel('directory/currency')->getConfigBaseCurrencies());

Database:

SQL queries before: 2
SQL queries after: 0
Output from method is in same format.

Manual testing scenarios `getConfigAllowCurrencies` (*)

**For this data in database: **

Call this code:

print_r(Mage::getModel('directory/currency')->getConfigAllowCurrencies());
print_r(Mage::getModel('directory/currency')->getConfigAllowCurrencies());

Output before changes:

Array
(
    [0] => BAM
    [2] => BGN
    [6] => CZK
    [9] => EUR
    [14] => HUF
    [15] => PLN
    [17] => PLN

    [18] => RON
    [20] => RSD
    [21] => RUB
    [22] => USD
)

Output afterchanges:

Array
(
    [0] => BAM
    [2] => BGN
    [6] => CZK
    [9] => EUR
    [14] => HUF
    [15] => PLN
    [18] => RON
    [20] => RSD
    [21] => RUB
    [22] => USD
)

Related SQL queries before:

Related SQL queries after:

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All automated tests passed successfully (all builds are green)

Update

Update from LTS

Updating from LTS repository

midlan · 2020-10-08T06:24:08Z

app/code/core/Mage/Directory/Model/Currency.php

+        $defaultCurrencies = (string) Mage::app()->getConfig()->getNode(self::XML_PATH_CURRENCY_BASE, 'default');
+        return [$defaultCurrencies];


why not change only the Mage_Directory_Model_Resource_Currency::getConfigCurrencies to use config (which is cached) ?

Agreed, there doesn't seem to be any good reason to directly query the config table so getConfigCurrencies could simply be:

/** * @deprecated */ public function getConfigCurrencies($model, $path) { return (string) Mage::app()->getConfig()->getNode($path, 'default'); }

you can't do this return explode(',', (string) Mage::app()->getConfig()->getNode($path, 'default'));, bcs. you can customize settings per website/store scope

app/code/core/Mage/Directory/Model/Currency.php

tmotyl · 2020-10-08T15:54:20Z

This sounds like a nice improvement.
What scenarios should be tested before the merge?

product , cart, checkout flow
switching currency by selector (the same store view)
switching currency in code using simulated environment
checkout when base currency is different with current
what else?

Sekiphp · 2020-10-09T22:31:44Z

@colinmollenhour @midlan

Or alternative solution (in my opinion us ugly, but complete without any additional SQL query):

    public function getConfigCurrencies($model, $path)
    {
        $result  = [];

        // default
        $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'default'))));

        // stores
        foreach (Mage::app()->getStores(true) as $store) {
            $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'stores', $store->getCode()))));
        }

        // websites
        foreach (Mage::app()->getWebsites(true) as $website) {
            $result = array_merge($result, explode(',', trim(Mage::app()->getConfig()->getNode($path, 'websites', $website->getCode()))));
        }

        sort($result);

        return array_unique($result);
    }

midlan · 2020-10-10T06:32:00Z

@Sekiphp I like it! I would only create variable for Mage::app()->getConfig()

Sekiphp · 2020-10-10T07:34:08Z

@midlan Yeah, I can do it, it can improve readability of code

Sekiphp · 2020-10-16T12:42:55Z

@midlan @colinmollenhour @tmotyl @FreeWall Final solution is here :-)

midlan

approved
worth to the note that the trim seems fixing an issue that occurred elsewhere (probably the config edit form)

colinmollenhour · 2020-10-20T14:22:25Z

Very nice. May I suggest a few improvements?

Use array_filter on the final result in case the config value is empty for one or more stores. The result of explode(',','') is [''] not [] so the empty string would end up as one of the elements in result.
Cache the result in case the method is called multiple times in one process. (maybe it never is in which case don't bother)
Skip inactive stores/websites unless there is a reason to keep currencies that are only active for disabled stores.

The base branch was changed.

github-actions · 2021-03-27T13:29:16Z

Unit Test Results

1 files ±0 1 suites ±0 0s ⏱️ ±0s
0 tests ±0 0 ✔️ ±0 0 💤 ±0 0 ❌ ±0
2 runs ±0 2 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit ca3d916. ± Comparison against base commit a4b82bf.

Sekiphp · 2021-04-07T07:24:38Z

@colinmollenhour Can you create issue for your suggestions?

Sekiphp and others added 6 commits January 17, 2020 15:59

Merge pull request #1 from OpenMage/1.9.4.x

201df92

Update

Merge pull request #2 from OpenMage/1.9.4.x

1a1ef13

Update from LTS

Merge remote-tracking branch 'upstream/1.9.4.x' into 1.9.4.x

d1a18ab

Merge pull request #3 from OpenMage/1.9.4.x

fc4dac6

Updating from LTS repository

Caching currencies

6826466

Caching currencies - base

f706c66

Sekiphp changed the title ~~Caching all allowed currencies~~ Directory currency - performance optimalization Oct 7, 2020

github-actions bot added the Component: Directory Relates to Mage_Directory label Oct 7, 2020

kiatng added the performance Performance related label Oct 8, 2020

FreeWall previously approved these changes Oct 8, 2020

View reviewed changes

midlan reviewed Oct 8, 2020

View reviewed changes

app/code/core/Mage/Directory/Model/Currency.php Outdated Show resolved Hide resolved

Fix currency load - without additional SQL queries

fa9ce02

Sekiphp dismissed FreeWall’s stale review via fa9ce02 October 16, 2020 12:39

Sekiphp added 2 commits October 16, 2020 14:41

Fix currency load - without additional SQL queries

fdd7d9b

Fix currency load - without additional SQL queries

f1d7345

FreeWall previously approved these changes Oct 16, 2020

View reviewed changes

midlan previously approved these changes Oct 16, 2020

View reviewed changes

Flyingmana added the hacktoberfest-accepted label Oct 21, 2020

Flyingmana changed the base branch from 1.9.4.x to 20.0 March 27, 2021 13:27

Flyingmana merged commit ca3d916 into OpenMage:20.0 Mar 27, 2021

Sekiphp deleted the performance/directory-default-currencies branch April 7, 2021 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directory currency - performance optimalization #1252

Directory currency - performance optimalization #1252

Sekiphp commented Oct 7, 2020 •

edited

Loading

midlan Oct 8, 2020

colinmollenhour Oct 8, 2020

Sekiphp Oct 9, 2020

tmotyl commented Oct 8, 2020

Sekiphp commented Oct 9, 2020 •

edited

Loading

midlan commented Oct 10, 2020

Sekiphp commented Oct 10, 2020 •

edited

Loading

Sekiphp commented Oct 16, 2020

midlan left a comment •

edited

Loading

colinmollenhour commented Oct 20, 2020

github-actions bot commented Mar 27, 2021

Sekiphp commented Apr 7, 2021

		$defaultCurrencies = (string) Mage::app()->getConfig()->getNode(self::XML_PATH_CURRENCY_BASE, 'default');
		return [$defaultCurrencies];

Directory currency - performance optimalization #1252

Directory currency - performance optimalization #1252

Conversation

Sekiphp commented Oct 7, 2020 • edited Loading

Description (*)

Manual testing scenarios getConfigBaseCurrencies (*)

Manual testing scenarios getConfigAllowCurrencies (*)

Contribution checklist (*)

midlan Oct 8, 2020

Choose a reason for hiding this comment

colinmollenhour Oct 8, 2020

Choose a reason for hiding this comment

Sekiphp Oct 9, 2020

Choose a reason for hiding this comment

tmotyl commented Oct 8, 2020

Sekiphp commented Oct 9, 2020 • edited Loading

midlan commented Oct 10, 2020

Sekiphp commented Oct 10, 2020 • edited Loading

Sekiphp commented Oct 16, 2020

midlan left a comment • edited Loading

Choose a reason for hiding this comment

colinmollenhour commented Oct 20, 2020

github-actions bot commented Mar 27, 2021

Unit Test Results

Sekiphp commented Apr 7, 2021

Sekiphp commented Oct 7, 2020 •

edited

Loading

Manual testing scenarios `getConfigBaseCurrencies` (*)

Manual testing scenarios `getConfigAllowCurrencies` (*)

Sekiphp commented Oct 9, 2020 •

edited

Loading

Sekiphp commented Oct 10, 2020 •

edited

Loading

midlan left a comment •

edited

Loading